Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Initial support for Scala 3 #858

Merged
merged 14 commits into from
Aug 6, 2024
Merged

Conversation

Johnnei
Copy link
Contributor

@Johnnei Johnnei commented Aug 1, 2024

Very minimal initial support for Scala 3 (LTS 3.3 and latest 3.4)

  • Split source into sharable components (reporting / configuration) and Scala 2/3 specific code
  • Set up InspectionTest that runs Scapegoat as true compiler plugin (with extra wrapping to extract the state without having to parse reports)
  • Set up base inspection traversal to help with extraction of snippets
  • Port OptionGet inspection to Scala 3

Some "obvious" omissions:

  • A lot of the Scala 2 generic traversal code is still missing so it might still traverse too aggressively
  • Because of that Suppression of warnings is still unsupported too

I didn't squash the commits to allow you to see my journey and maybe help a bit with reviewing the steps I took. But please squash on merge :)

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 92.70833% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.45%. Comparing base (fa90063) to head (67705b6).
Report is 27 commits behind head on master.

Files Patch % Lines
...c/main/scala-3/com/sksamuel/scapegoat/Plugin.scala 93.02% 3 Missing ⚠️
...scala-3/com/sksamuel/scapegoat/FeedbackDotty.scala 85.71% 1 Missing ⚠️
...3/com/sksamuel/scapegoat/InspectionTraverser.scala 66.66% 1 Missing ⚠️
.../scala/com/sksamuel/scapegoat/InspectionBase.scala 50.00% 1 Missing ⚠️
...rc/main/scala/com/sksamuel/scapegoat/Warning.scala 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #858      +/-   ##
==========================================
- Coverage   88.08%   82.45%   -5.64%     
==========================================
  Files         142       19     -123     
  Lines        1578      302    -1276     
  Branches      278       32     -246     
==========================================
- Hits         1390      249    -1141     
+ Misses        188       53     -135     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.scalafmt.conf Show resolved Hide resolved
@@ -22,15 +19,16 @@ developers := List(
)
)

scalaVersion := "2.13.14"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if this is too optimistic (especially for codecov?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov seemed to work fine on this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I meant more for coverage itself. Most of the coverage is on the rules which aren't there for Scala 3 yet. Not sure what test coverage we want to highlight here (or maybe somehow merge a report from 2.12 and 2.13?)

Copy link
Collaborator

@saeltz saeltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your effort! This looks great. I have added some comments.

build.sbt Outdated Show resolved Hide resolved
src/main/scala-3/com/sksamuel/scapegoat/Plugin.scala Outdated Show resolved Hide resolved
.github/workflows/pr-checks.yml Show resolved Hide resolved
@@ -22,15 +19,16 @@ developers := List(
)
)

scalaVersion := "2.13.14"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov seemed to work fine on this PR.

src/main/scala-3/com/sksamuel/scapegoat/Inspections.scala Outdated Show resolved Hide resolved
src/main/scala-3/com/sksamuel/scapegoat/Plugin.scala Outdated Show resolved Hide resolved
src/main/scala/com/sksamuel/scapegoat/CorePlugin.scala Outdated Show resolved Hide resolved
@Johnnei
Copy link
Contributor Author

Johnnei commented Aug 6, 2024

@saeltz Could you do another round of review? I think should be minimum viable now to get Scala 3 support going.

Slight decrease in project codecov which I think is mostly the Scala 2.13 version checks which don't trigger in Scala 3.

Copy link
Collaborator

@saeltz saeltz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a ton!
Let's get this merged and add more and more inspections for Scala 3 iteratively.

@saeltz saeltz merged commit b282044 into scapegoat-scala:master Aug 6, 2024
14 of 15 checks passed
@saeltz
Copy link
Collaborator

saeltz commented Aug 6, 2024

@mccartney, would you issue a new release? Maybe even a minor version? We should also properly announce this.

@t1b00
Copy link

t1b00 commented Aug 12, 2024

Hey, really cool to see that you are considering a port to Scala 3.
As part of my semester project, I actually looked into porting Scapegoat to Scala 3 as this was an issue for our project and for the Scala community. However, I chose to use the Scalafix tool developed by the Scalacenter as it was compatible with Scala 3 instead of trying to port the code itself (which also fit more into my learning goals).
My semester project has ended but I am actively continuing to work on it.
Would you be interested in collaborating? I've ported 36 rules for now, with functioning tests. Maybe that could shave off some workload for you ;) https://github.com/dedis/scapegoat-scalafix

@mccartney
Copy link
Collaborator

@Johnnei wow, this is great! What a nice surprise.
@saeltz Sorry, I must've missed the notification.

Will perform the release and will report back. I think it should be a major one, even if there's no specific backward incompatibility we are aware of.

@mccartney
Copy link
Collaborator

3.0.0 is on its way.

@Johnnei Johnnei deleted the scala-3 branch August 13, 2024 13:22
@Johnnei
Copy link
Contributor Author

Johnnei commented Aug 13, 2024

Hey, really cool to see that you are considering a port to Scala 3. As part of my semester project, I actually looked into porting Scapegoat to Scala 3 as this was an issue for our project and for the Scala community. However, I chose to use the Scalafix tool developed by the Scalacenter as it was compatible with Scala 3 instead of trying to port the code itself (which also fit more into my learning goals). My semester project has ended but I am actively continuing to work on it. Would you be interested in collaborating? I've ported 36 rules for now, with functioning tests. Maybe that could shave off some workload for you ;) https://github.com/dedis/scapegoat-scalafix

@t1b00 Cool to see this interest!
It would be great to have more contributors to get Scala 3 support expanded.

I do think that using Scalameta trees is a nice step forward, as it should (as far I understand) enable more code re-use between Scala 2 and Scala 3 inspections.

However, I do think that Scapegoat's reporting is one aspect that wouldn't hold as custom Scalafix rules.
One of the hard things with Static Analysis is adoption on existing projects and changing rulesets over time.
With Scapegoat's customizable Rule Severity, projects can gradually adopt a more enforcing ruleset without breaking the build.

This is something that helped me to scale this to hundreds of projects in my company in an "advising" mode, rather than build breaker.

To me, one thing that I really like about Scalafix is the capability that it can fix its own findings.
While for plenty of static analysis rules, there's no generic automatic fix.
Only an advise on how to approach the solution.
I'd suspect that it would be quite a lessened experience compared to Scalafix' out-of-the-box rules.

So I'm quite interested to see if incorporating Scalameta is feasible, but not sure relying fully on Scalafix will provide the best experience.

@mccartney @saeltz Interested to hear your thoughts on this as well.

@t1b00
Copy link

t1b00 commented Aug 14, 2024

@t1b00 Cool to see this interest! It would be great to have more contributors to get Scala 3 support expanded.

I do think that using Scalameta trees is a nice step forward, as it should (as far I understand) enable more code re-use between Scala 2 and Scala 3 inspections.

However, I do think that Scapegoat's reporting is one aspect that wouldn't hold as custom Scalafix rules. One of the hard things with Static Analysis is adoption on existing projects and changing rulesets over time. With Scapegoat's customizable Rule Severity, projects can gradually adopt a more enforcing ruleset without breaking the build.

This is something that helped me to scale this to hundreds of projects in my company in an "advising" mode, rather than build breaker.

To me, one thing that I really like about Scalafix is the capability that it can fix its own findings. While for plenty of static analysis rules, there's no generic automatic fix. Only an advise on how to approach the solution. I'd suspect that it would be quite a lessened experience compared to Scalafix' out-of-the-box rules.

So I'm quite interested to see if incorporating Scalameta is feasible, but not sure relying fully on Scalafix will provide the best experience.

@mccartney @saeltz Interested to hear your thoughts on this as well.

Hey, thanks for your answer!

Let me clarify a few things about Scalafix and Scalameta since they are a bit opaque and I've acquired quite a bit of experience overtime with them.

"With Scapegoat's customizable Rule Severity, projects can gradually adopt a more enforcing ruleset without breaking the build."

That's also the case in Scalafix! Rules can have a severity of Info, Warning or Error, with only error causing compilation errors. Furthermore, it's trivial to activate / deactive rules as one can simply list them in their Scalafix configuration file.

One thing that I really like about Scalafix is the capability that it can fix its own findings.

It can, if you write rewrite rules for it. Scalafix is not a linter, but a linter building tool and it was designed for the community to build their own rule sets, not with the idea of it becoming a linter :)
For my linter project, I actually focused on the linting part, not rewriting, because as you are saying:

While for plenty of static analysis rules, there's no generic automatic fix.

Scalafix allows you to write linter rules or rewrites, but you can simply stick to linter rules if you feel that there is no standard rewrite. Though I could extend some of the rules (e.g. MapGetAndGetOrElse) to a rewrite.

I'd be really happy to collaborate with you and the rest of the Scapegoat team on this if you feel like porting to Scalafix and Scalameta is in your interest.

@saeltz
Copy link
Collaborator

saeltz commented Aug 18, 2024

I'd be really happy to collaborate with you and the rest of the Scapegoat team on this if you feel like porting to Scalafix and Scalameta is in your interest.

I'd be happy to see this happening. Anything that brings us closer to full Scala 3 support is desirable.

@t1b00
Copy link

t1b00 commented Aug 20, 2024

I'd be happy to see this happening. Anything that brings us closer to full Scala 3 support is desirable.

Amazing! I think adding my code to the Scapegoat repo would be quite cumbersome as the setup is quite different, but you could definetly indicate this as the WIP version for Scapegoat for Scala 3 if that suits you. Otherwise we can also discuss different modalities.
Feel free to contact me by email: [email protected]

@t1b00 t1b00 mentioned this pull request Sep 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants